Skip to content

Conversation

@prajwal-gawande492
Copy link
Contributor

No description provided.

@ppc64le-cloud-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: prajwal-gawande492
Once this PR has been reviewed and has the lgtm label, please assign yussufsh for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ppc64le-cloud-bot ppc64le-cloud-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 13, 2025
@prajwal-gawande492 prajwal-gawande492 force-pushed the node-autoscaling-ipi branch 2 times, most recently from fb1ae5d to 78d39a1 Compare August 14, 2025 06:37
delayAfterAdd: 10m
delayAfterDelete: 5m
delayAfterFailure: 30s
delayAfterFailure: 30s

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate Line

Suggested change
delayAfterFailure: 30s

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved


- name: Wait for terminating the busybox pods
wait_for:
timeout: 180

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use loop until conditions are met (until: with retries and delay) instead of fixed waits.
Look for other places also!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a loop until condition.

Comment on lines +81 to +85
- name: Create a namespace and label it to deploy busybox
kubernetes.core.k8s:
name: test
api_version: v1
kind: Namespace
state: present

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add cleanup for 'test' namespace at the end of the play to avoid leftover resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

block:
- name: Check the machine count before autoscaling
shell: oc get machineset -n openshift-machine-api -o=jsonpath='{.items[0].status.replicas}'
register: machine_replicas

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
register: machine_replicas
register: initial_machine_count

We can have self-explanatory and clearer variable names

machine_replicas      -> initial_machine_count
machines_count        -> post_scaleup_machine_count
machineset_replicas   -> pre_scaledown_machine_count

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved.

Signed-off-by: prajwal-gawande492 <[email protected]>
@aman4433
Copy link

aman4433 commented Sep 3, 2025

/lgtm

@ppc64le-cloud-bot ppc64le-cloud-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants